Durga js window objects api#540
Conversation
Summary of ChangesHello @Saidurgasatturi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the codebase by adding a series of new assignments and examples designed to illustrate core client-side web development concepts. The primary focus is on practical applications of the JavaScript Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of assignments demonstrating various features of the browser's window object and storage APIs. The examples cover navigation, dimensions, scrolling, timers, cookies, local/session storage, and inter-window communication. My review focuses on improving code quality, security, and maintainability. Key areas for improvement include adhering to best practices by separating HTML, CSS, and JavaScript; enhancing user experience by replacing alert() calls with non-blocking UI feedback; fixing a critical bug in cookie parsing; and addressing significant security vulnerabilities in the use of postMessage by specifying target origins and verifying message origins.
| const iframe = document.getElementById("outerFrame"); | ||
| document.getElementById("sendToIframe").addEventListener("click", () => { | ||
| const msg = document.getElementById("mainMsg").value.trim(); | ||
| iframe.contentWindow.postMessage(msg || "No message", "*"); |
There was a problem hiding this comment.
Security Vulnerability: Using "*" as the targetOrigin for postMessage is dangerous. You must specify the exact origin of the iframe to prevent malicious sites from intercepting your messages.
| window.onmessage = (event) => { | ||
| document.getElementById("mainReceived").textContent = event.data; | ||
| }; |
There was a problem hiding this comment.
Security Vulnerability: The onmessage handler must validate the sender's origin via event.origin. Without this check, your page is vulnerable to receiving and processing malicious data from any website.
| window.onmessage = (event) => { | ||
| document.getElementById("iframeReceived").textContent = event.data; | ||
| }; |
There was a problem hiding this comment.
Security Risk: The onmessage handler is missing an origin check. You must verify event.origin to ensure that you are only processing messages from the expected parent window.
| // Send message to top-most window | ||
| document.getElementById("sendToTop").addEventListener("click", () => { | ||
| const msg = document.getElementById("iframeMsg").value.trim(); | ||
| window.top.postMessage("From iframe: " + (msg || "No message"), "*"); |
There was a problem hiding this comment.
Security Risk: postMessage is being used with a wildcard targetOrigin ("*"). This is unsafe. You should specify the exact origin of the top window to ensure messages are not exposed to other sites.
| }; | ||
| document.getElementById("sendToParent").addEventListener("click", () => { | ||
| const message = document.getElementById("iframeMsg").value.trim(); | ||
| window.parent.postMessage(message || "No message", "*"); |
There was a problem hiding this comment.
Using "*" as the targetOrigin for postMessage is a significant security risk. It allows any website to listen for the message you're sending. You should always specify the exact origin of the window you intend to communicate with to prevent data from being intercepted by malicious sites.
| <style> | ||
| body { text-align: center; font-family: Arial; } | ||
| iframe { width: 80%; height: 150px; border: 2px solid blue; margin: 10px; } | ||
| input, button { padding: 6px; margin: 4px; } | ||
| </style> |
There was a problem hiding this comment.
To follow best practices, CSS should be placed in external files rather than in an inline <style> block. This helps with code organization and reusability.
| <script> | ||
| // Receive message from main | ||
| window.onmessage = (event) => { | ||
| document.getElementById("iframeReceived").textContent = event.data; | ||
| }; | ||
|
|
||
| // Send message to top-most window | ||
| document.getElementById("sendToTop").addEventListener("click", () => { | ||
| const msg = document.getElementById("iframeMsg").value.trim(); | ||
| window.top.postMessage("From iframe: " + (msg || "No message"), "*"); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
This inline script should be extracted into its own .js file to maintain a clean separation between your page's structure (HTML) and its behavior (JavaScript).
| <label for="yInput">Y:</label> | ||
| <input type="number" id="yInput" placeholder="Enter Y value"> | ||
|
|
||
| <button onclick="scrollToPosition()">Scroll to (X, Y)</button> |
There was a problem hiding this comment.
Using inline onclick attributes mixes HTML structure with JavaScript logic, which goes against modern web development best practices and the guidelines in your PR description. It's better to attach event listeners in your JavaScript file using addEventListener. This improves separation of concerns and makes the code easier to maintain.
| <style> | ||
| body { | ||
| font-family: Arial, sans-serif; | ||
| margin: 0; | ||
| padding: 0; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| height: 100vh; | ||
| background: #f0f0f0; | ||
| } | ||
| .box { | ||
| width: 150px; | ||
| height: 150px; | ||
| background: #3498db; | ||
| border-radius: 10px; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| color: white; | ||
| font-weight: bold; | ||
| animation: bounce 2s infinite; | ||
| } | ||
| @keyframes bounce { | ||
| 0%, 100% { | ||
| transform: translateY(0); | ||
| } | ||
| 50% { | ||
| transform: translateY(-50px); | ||
| } | ||
| } | ||
| @media (max-width: 768px) { | ||
| .box { | ||
| width: 120px; | ||
| height: 120px; | ||
| font-size: 14px; | ||
| background: #e67e22; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 480px) { | ||
| .box { | ||
| width: 90px; | ||
| height: 90px; | ||
| font-size: 12px; | ||
| background: #2ecc71; | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
The CSS for this page is embedded within a <style> tag in the HTML file. According to the best practices outlined in the PR description, it's better to move CSS to external files. This improves code organization, reusability, and maintainability by separating concerns.
| @@ -0,0 +1,35 @@ | |||
| let newWindow; | |||
There was a problem hiding this comment.
Using a global variable (newWindow) can lead to issues in larger applications, such as naming conflicts. It's a better practice to encapsulate state within a module or an object to avoid polluting the global scope.
1 participant
Terms and Conditions
HTML Best Practices
File Naming Convention:
Follow consistent and descriptive naming (e.g., dashboard.html, user-profile.html).
Use lowercase letters and hyphens instead of spaces.
Page Title:
Ensure the <title> tag is descriptive and aligns with the page content.
Include meaningful keywords for SEO if applicable.
Semantic Markup:
Use appropriate tags like <header>, <footer>, <section>, <article> for better readability and accessibility.
Accessibility Standards:
Ensure the use of alt attributes for images and proper labels for form elements.
Use ARIA roles where necessary.
Validation:
Ensure the code passes HTML validation tools without errors or warnings.
Structure and Indentation:
Maintain consistent indentation and proper nesting of tags.
Attributes:
Ensure all required attributes (e.g., src, href, type, etc.) are correctly used and not left empty.
CSS Best Practices
File Organization:
Use modular CSS files if applicable (e.g., base.css, layout.css, theme.css).
Avoid inline styles unless absolutely necessary.
Naming Conventions:
Use meaningful class names following BEM or other conventions (e.g., block__element--modifier).
Code Reusability:
Avoid duplicate code; use classes or mixins for shared styles.
Responsive Design:
Ensure proper usage of media queries for mobile, tablet, and desktop views.
Performance Optimization:
Minimize the use of unnecessary CSS selectors.
Avoid overly specific selectors and ensure selectors are not overly deep (e.g., avoid #id .class1 .class2 p).
Consistency:
Follow consistent spacing, indentation, and use of units (rem/em vs. px).
Maintain a single coding style (e.g., always use double or single quotes consistently).
Javascript Best Practices
File Organization:
Ensure scripts are modular and logically separated into files if needed.
Avoid mixing inline JavaScript with HTML.
Logic Optimization:
Check for redundancy and ensure the code is optimized for performance.
Avoid unnecessary API calls or DOM manipulations.
Solution Approach:
Confirm that the code solves the given problem efficiently.
Consider scalability for future enhancements.
Readability:
Use clear variable and function names.
Add comments for complex logic or algorithms.
Error Handling:
Ensure proper error handling for API calls or user input validation.
Code Quality:
Check for potential bugs (e.g., missing await, mishandling of null/undefined values).
Avoid unnecessary console.log statements in production code.
Security:
Avoid hardcoding sensitive data.
Sanitize user input to prevent XSS and other vulnerabilities.
Best Practices:
Use const and let instead of var.
Follow ES6+ standards where applicable.